fix(datagrid): read the inspector row from the grid that owns the selection - #1998
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
datlechin
force-pushed
the
fix/structure-inspector-row-source
branch
from
July 30, 2026 12:38
4dc9334 to
9744047
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Selecting a row in the Structure tab showed the data row in the same position, not the structure row.
Root cause
GridSelectionStateis one selection channel written by three grids (TableStructureView.swift:92,CreateTableView.swift:98, and the data grid), and it carries no owner. The inspector resolved those indices against the data tab's rows unconditionally, so a structure selection rendered a data row.The write path was worse.
isSidebarEditableonly checked "table tab + non-empty selection", so the inspector rendered an editable data form over a structure selection,onFieldChangedrecorded a data cell change keyed by the structure grid's display index, andsaveSidebarEditswouldUPDATEa data row the user never selected.Every other consumer of the channel already discriminated on
resultsViewMode(MainContentCommandActions.saveChanges, add, delete, copy). The inspector was the one that never asked.The fix
GridSelectionOwneris now the single answer to "whose display positions are these", resolved from the same(tabType, resultsViewMode)pair the save routing uses. The grid that owns a schema selection publishes its row throughInspectorRowSourceinstead of the inspector guessing:StructureGridDelegateandCreateTableGridDelegateconform, andStructureInspectorRowBuilderbuilds the payload from the sameStructureRowProviderthe grid renders, so the panel and the grid cannot disagree about what a row holds.dataGridDidEditCell, so an inspector edit is indistinguishable from an inline cell edit: sameStructureChangeManager, one undo step, one pending change, applied by the same Save.updateColumnregisters one undo action per call. Per-keystroke commits would make Cmd+Z undo one character at a time.supportsSchemaEditing, gets read-only fields. Editability mirrors the grid'scanEditexactly, so the inspector is never more permissive than the grid beside it.Also fixed, same bug class
duplicateRow()andcopySelectedRowsWithHeaders()acted on data rows while the Structure tab was open. Both now require the data grid to own the selection.saveSidebarEditsalso stopped indexingtableRows.rows[rowIndex]with a display position, which violated the selection-indices invariant in CLAUDE.md.Tests
GridSelectionOwnerTests: the resolve matrix, including query tabs, JSON view, and tabs with no row grid.StructureInspectorRowBuilderTestsandStructureGridDelegateInspectorTests: the regression. The payload comes from the structure provider, and an edit made under an active filter or sort lands on the entity behind that display row rather than the one at that array position.StructureRowProviderTests(modified fields and pending deletes across columns, indexes, and foreign keys) andMultiRowEditStateTests(schema field descriptions, field identity across a reconfigure so an editor keeps focus, commit routing).No
TableProUITestscoverage: the flow needs a live connection with a loaded table, so it would not run deterministically in CI.Notes for the reviewer
Localizable.xcstringsis deliberately not in this PR. The working copy held unrelated regeneration churn from a local build.Choose Typeis the one new string and the next build will add it.hasUnsavedWorkcovers data rows, files, and users and roles only. Inspector edits land in the same change manager as grid edits, so this behaves identically either way.